Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make offsetGridLines behavior consistent and add new offset option to scales #4545

Merged
merged 4 commits into from
Aug 14, 2017

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Jul 21, 2017

Document says

offsetGridLines

If true, the bars for a particular data point fall between the grid lines. If false, the grid line will go right down the middle of the bars.

However, the current implementation doesn't change the positions of grid lines, and bars stay between the grid lines regardless of whether it is true or false. Instead, only the positions of labels are shifted.

When a chart has mixed data types (e.g. bar and line) and offsetGridLines is false, the grid line goes right down the middle of the bars as the document explains. This seems to me the correct behavior even for non-mixed charts when offsetGridLines is false.

Another problem is that the right most bar is completely hidden (in case of a bar chart) or bars at the both edges are hidden partially (in case of a mixed chart) when offsetGridLines is false. Calculation of the bar width is also incorrect.

screen shot 2017-07-19 at 6 31 18 pm

In case of time scale charts, offsetGridLines has no effect, and bars are drawn as if it were set to true. It makes more sense to me that the center of a bar is positioned at the time that the data point refers as the default behavior. The behavior of the offsetGridLines option should be consistent across different scales.

The other issue is that the width of bars and padding are calculated based on the number of the ticks. If the number of the samples are fewer or more than the number of the ticks, unnecessary margin or overlaps will appear.

screen shot 2017-07-22 at 12 03 47 am

This PR contains the following changes to resolve the issues above.

  • Remove chart.isCombo
  • Remove the includeOffset argument from getPixelForValue() and getPixelForTick()
  • Add a new offset option to scales to add extra space (half tick interval) at edges
  • Bar controller automatically calculates the bar width to avoid overlaps
  • When offsetGridLines is true, grid lines move to the left by one half of the tick interval, and labels don't move
  • Add tests to the scales and the bar controller

The following images compare the current version (on the left) and the proposed version (on the right).

screen shot 2017-08-12 at 3 09 57 am

Below are charts with time scales.

screen shot 2017-08-12 at 3 10 34 am

screen shot 2017-08-12 at 3 11 06 am

Below are examples of horizontal bar charts.

screen shot 2017-08-12 at 3 11 47 am

Below are examples of non-linear time scales.

screen shot 2017-08-12 at 3 22 14 am

Fixes #2323
Fixes #2415
Fixes #3263
Fixes #3297
Fixes #3579
Fixes #3613
Fixes #3682
Fixes #3913
Fixes #4051
Fixes #4343
Fixes #4471
Fixes #4560

@etimberg
Copy link
Member

@nagix thanks for the incredibly detailed PR notes. I will try and look at the code soon.

@@ -46,6 +46,11 @@ module.exports = function(Chart) {
var labels = me.getLabels();
// If we are viewing some subset of labels, slice the original array
me.ticks = (me.minIndex === 0 && me.maxIndex === labels.length - 1) ? labels : labels.slice(me.minIndex, me.maxIndex + 1);

// Disable zero line if a chaer has bars and offsetGridLines is false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo chaer

@@ -281,6 +281,29 @@ module.exports = function(Chart) {
max = ticks.length ? ticks[ticks.length - 1] : max;
}

// If the chart has bars, shift min and max based on the sample size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll need a different logic here if mode: series

ticks.mode ('linear'(default)|series): series displays ticks at the same distance from each other, whatever the time value they represent, while linear displays them linearly in time: the distance between each tick represent the amount of time between their time values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! As each tick interval can have different value within the same scale, the sample size of bars can also very. So, I removed the ruler code from controller.bar.js, and made calculateBarIndexPixels() call getBarPixelsForIndex() to get the left and right edge of the bar for a specified index. I also modified the calculation logic in scale.time.js to support ticks.mode and ticks.source.

// If the chart has bars, shift min and max based on the sample size
if (me.options.gridLines.offsetGridLines || me.chart.hasBar) {
var sampleSizeMs = moment.duration(me.options.sampleSize || 0).asMilliseconds();
var offsetGridLines = me.options.gridLines.offsetGridLines;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you set this variable outside the if then you can use it in the if check

@@ -116,6 +117,13 @@ options = {
}
```

### sampleSize
If specified, the width for each sample will be calcurated based on this value. If not specified, it will be the available width (the space between the gridlines). This only works if `barThickness` is not set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcurated -> calculated

@nagix
Copy link
Contributor Author

nagix commented Jul 23, 2017

Below are the time scale charts with gridLines.offsetGridLines = true, tick.mode = linear | series and tick.source = auto | labels. It looks ok now. There are overlaps when gridLines.offsetGridLines is false, but that problem cannot be solved when tick time and sample time are not aligned, and the fixed sample size (sampleSize option) should be used in that case.

screen shot 2017-07-24 at 2 30 44 am

@benmccann
Copy link
Contributor

benmccann commented Jul 24, 2017

I think there's a bug here with mode: series and offsetGridLines: false. See the screenshot below. It causes the bars to be different widths.

screenshot from 2017-07-23 23-07-27

@benmccann
Copy link
Contributor

benmccann commented Jul 24, 2017

@nagix I want to thank you for the truly awesome description and your attention to testing. We really appreciate all the time you've put into fixing this issue. This is a very important issue with a lot of benefit given how many times people have reported it

I think there's one main issue, which is that it's making many other classes aware of the bar chart when they shouldn't need to know about the bar chart. It would be preferable to keep the fix contained within the bar chart as much as possible. If the bar chart looks at the ticks of the axis hopefully it should be possible to calculate the bounds of the bar

@nagix
Copy link
Contributor Author

nagix commented Jul 24, 2017

@benmccann Thanks for reviewing this, and I understand your concern. To make it simpler, how about the following idea?

  • Don't use sampleSize option
  • The default bar size is the size of the tick interval that the data sample is on
    • In case of a category scale, it will be a fixed size. In case of a time scale, it will depend on the tick configuration
  • When detecting overlap with the next bars, recalculate the size individually so that they fit each other

@benmccann
Copy link
Contributor

That sounds reasonable to me. @simonbrunel thoughts?

Simon and I were talking about the one point below

The default bar size is the size of the tick interval that the data

I think we came to the conclusion that it might be nice to have an option that switches this between two modes. One is the mode you mentioned. The other is a mode where you force the bars to be equally sized (the size of the closest two ticks). Here's a use case for what I'm talking about: imagine you're plotting a daily stock with a bar chart. You set mode:linear. I think you would want five equally sized bars with a space for the weekend followed by five equally sized bars for the next week. In this case it would be strange if the bars for Monday and Friday were much bigger than the bars for Tue, Wed, Thurs

@benmccann
Copy link
Contributor

Here's a sample that can be used to reproduce the bug I mentioned above when mode: series and offsetGridLines: false: https://raw.githubusercontent.com/benmccann/Chart.js/offsetGridlineExample/samples/scales/time/financial.html

@benmccann
Copy link
Contributor

Also, I haven't tested with your PR, but currently on master, if you try to plot a bar on a time scale, there's one point that doesn't get plotted (I'm not sure if it's the first or the last). That might be something else we want to test for

@nagix
Copy link
Contributor Author

nagix commented Jul 28, 2017

I have changed the code so that the bar controller and the scales don't depend on each other as much as possible. Now bar controller calculates the bar width based on the return value of the getPixelForValue function.

  • Remove sampleSize option
  • Calculate the grid line offset based on the minimum interval between samples
  • Add code to support ticks.mode and ticks.source option in the time scale
  • Modify the interpolation table to support lookup for tick indexes
  • Adjust the range of scales so that all bars can be displayed

@nagix
Copy link
Contributor Author

nagix commented Jul 28, 2017

@benmccann The issue you reported has been fixed. Also, the last bar is plotted on a time scale, which was reported in #3297.

screen shot 2017-07-28 at 6 29 50 am

@simonbrunel
Copy link
Member

@nagix can you squash and rebase?

@simonbrunel
Copy link
Member

Thanks @nagix for this awesome work. I think there is still one (implementation) issue, can you post a screenshot of bars on a time scale with mode: 'linear', source: 'labels', with labels at irregular times:

image

@benmccann
Copy link
Contributor

@nagix will you be able to squash and rebase to get the latest commits from master to stop showing up in your PR? i'm very excited for this improvement to make its way in!

@nagix
Copy link
Contributor Author

nagix commented Aug 5, 2017

Sorry for the late response. I did squash and rebase to the latest master.

I have also added several tests to the time scale, category scale and bar controller to cover the ticks.offset, barThickness and maxBarThickness options.

When barThickness is not specified, the bar width will be sized based on the minimum interval between bars in the chart to avoid overlaps, and all bars will have the same width. When 'max' is set to barThickness, the bar width will be maximized individually, and each bar can have a different width.

@nagix nagix changed the title Make offsetGridLines behavior consistent and add new sampleSize option Make offsetGridLines behavior consistent and add new ticks.offset option Aug 5, 2017
@@ -31,6 +31,7 @@ The following options are common to all cartesian axes but do not apply to other
| `maxRotation` | `Number` | `90` | Maximum rotation for tick labels when rotating to condense labels. Note: Rotation doesn't occur until necessary. *Note: Only applicable to horizontal scales.*
| `minRotation` | `Number` | `0` | Minimum rotation for tick labels. *Note: Only applicable to horizontal scales.*
| `mirror` | `Boolean` | `false` | Flips tick labels around axis, displaying the labels inside the chart instead of outside. *Note: Only applicable to vertical scales.*
| `offset` | `Boolean` | `false` | If true, extra space is added to the both edges and the axis is scaled to fit into the chart area. This is set to `true` in the bar chart by default.
Copy link
Contributor

@benmccann benmccann Aug 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds quite similar to the scale.bounds feature that was implemented in #4556 (docs are being added in #4592). Is it possible to use that option? I'm not quite sure if this is doing the same thing or not

The scale.bounds feature is only in the time scale right now, but there's a desire to add it to the other scales as well, so that might be better than creating a new option if possible

Copy link
Contributor

@benmccann benmccann Aug 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a couple screenshots to show the scale.bounds option

scale.bounds: 'data'

bounds-data

scale.bounds: 'ticks'

bounds-ticks

I think bounds: 'ticks' is similar to offset: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When scale.bounds: 'ticks' but the first and last data points are on the ticks, the scale won't add extra spaces at edges (see 5 below). However, offset: true will always add them to make sure that the whole bar rectangles are displayed (see 7 below).

screen shot 2017-08-06 at 10 34 06 am

screen shot 2017-08-06 at 10 34 13 am

Internally, offset: true doesn't change the time/pos lookup table. It just add one half size of the next tick intervals in getPixelForValue() (or getPixelForOffset() in case of a time scale), and this can be easily applied to other scales such as a category scale.

@benmccann
Copy link
Contributor

Thanks @nagix! There are some great changes here!

curr = scale.getPixelForValue(null, i, datasetIndex);
data.push(curr);
minInterval = Math.min(minInterval, curr - prev);
if (ticksOffset) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this if/else? Could you simplify by just always checking that curr > min && curr < max?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle an edge case like that a dataset contains only one data point that is on the edge of a scale, the scale has only one tick and ticks.offset: false. In this case, we cannot calculate the bar width based on a tick interval or the distance from the nearest edge (because it is on the edge), so it uses the distance from the farthest edge.

@andig
Copy link
Contributor

andig commented Aug 9, 2017

Checking in here thanks to @etimberg. Amazing PR! Could you kindly share any fiddle that shows a combined bar/line chart, preferably with lines using x/y notation instead of labels?

… scales

- Remove `chart.isCombo`
- Remove the includeOffset argument from `getPixelForValue()` and `getPixelForTick()`
- Add a new `offset` option to scales to add extra space at edges
- Bar controller automatically calculates the bar width to avoid overlaps
- When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move
- Add tests to the scales and the bar controller
@simonbrunel
Copy link
Member

@nagix is this PR ready to be merged?

@nagix
Copy link
Contributor Author

nagix commented Aug 13, 2017

@simonbrunel Yes, it is ready.

@simonbrunel simonbrunel merged commit 7dc71d0 into chartjs:master Aug 14, 2017
@benmccann
Copy link
Contributor

Woo! This is great! Thanks so much @nagix!

Will you be submitting a PR to add barThickness: 'min' next?

@maxwell
Copy link

maxwell commented Aug 14, 2017

yay! Thank you! Do you think this would warrant a new release?

@myadav-etindia
Copy link

@nagix
Just wanted to confirm if all the fixes done as part of this PR are available in chart.js release. We are using version 2.6.0 and that does not contain these fixes.
By when these fixes will be available ?

Thanks

@andig
Copy link
Contributor

andig commented Aug 18, 2017

@myadav-etindia this PR is in the master branch and will probably released with 2.7.0. See http://www.chartjs.org/docs/master/developers/ for how to get access to the development releases (= master branch).

@myadav-etindia
Copy link

@andig
Thanks. Got the latest.

silverspectro pushed a commit to silverspectro/Chart.js that referenced this pull request Aug 21, 2017
…hartjs#4545)

Add a new `offset` option to scales to add extra space at edges and remove the `includeOffset` argument from `getPixelForValue()` and `getPixelForTick()`. The bar controller now automatically calculates the bar width to avoid overlaps. When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move.
brewingcode added a commit to brewingcode/trip-track that referenced this pull request Aug 23, 2017
@myadav-etindia
Copy link

@andig
Tentatively by when would the chart.js 2.7.0 release be available ?

@andig
Copy link
Contributor

andig commented Sep 1, 2017

@myadav-etindia I wouldn't know really- it's a question for the maintainers. My feeling is though that there is still a lot of movement that would not warrant a realease in the near future. You could still use a snapshot of the current master right now.

See #4706

@buddyp450
Copy link

My team previously extended elements.rectangle because the existing Rectangle element assumed that the 'x' was the central point whose x1 and x2 were calculated dividing the width by 2. Our extended element assumes that x is the start and x + width = x2. I'll need to check this out to see if this is even needed anymore but I'm assuming it's not which would be a huge boon for us.

@Offlein
Copy link

Offlein commented Oct 25, 2017

I was really pleased with the work done here, but one thing I'm confused about is the intention of how this handles time series line charts.

I have daily data that I'd like to present with labels grouped and labeled by month. That is, the gridLine should appear at the first of the month, but I'd expect the tick label (say "Sep" or "Jan") should appear centered between the two. It seems like with offsetGridLines: true, it appears visually the way one would expect, but the gridlines correspond to the halfway point in the month.

This fiddle illustrates what I mean: https://jsfiddle.net/y152r1nh/4/

Hovering over the point on the line before the label "Jan" gives me dates for "December 15". Am I misunderstanding how to use this feature, or is that an oversight?

@benmccann
Copy link
Contributor

This feature is moving the gridlines for bar charts. I.e. if you're drawing a bar chart and don't want the grid line through the middle of the bar then move the gridlines over. What's you're asking for is a feature probably more like offsetLabels: true

@Offlein
Copy link

Offlein commented Oct 27, 2017

@benmccann Thanks. So that just plain doesn't exist, huh? I guess I'll look into how to implement such a thing.

@flausinoth
Copy link

@nagix What if you have an interval of days? for example: from day 20 to day 23. When setting the values, the bar graph distorts.

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
…hartjs#4545)

Add a new `offset` option to scales to add extra space at edges and remove the `includeOffset` argument from `getPixelForValue()` and `getPixelForTick()`. The bar controller now automatically calculates the bar width to avoid overlaps. When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move.
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
…hartjs#4545)

Add a new `offset` option to scales to add extra space at edges and remove the `includeOffset` argument from `getPixelForValue()` and `getPixelForTick()`. The bar controller now automatically calculates the bar width to avoid overlaps. When `offsetGridLines` is true, grid lines move to the left by one half of the tick interval, and labels don't move.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment